Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dev: Document astyle python script #6292

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Ryanf55
Copy link
Contributor

@Ryanf55 Ryanf55 commented Sep 21, 2024

Document the astyle format script, and how we can ignore format commits in the git blame.

@@ -21,8 +21,15 @@ automatically according to the guidelines below.

astyle --options=Tools/CodeStyle/astylerc <filename>

Some files are enforced to be *astyle* clean. To format them automatically, run:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure we want to push users to run this all the time when only a few files are affected,,,,maybe we should specify that it should be checked if you have modified the following ..... libraries or files?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More maintenance burden, @Hwurzburg ?

We should probably put this advice - run astyle - into the CI error message when one of these files fails the checks, if we haven't already. It's much more likely to be useful there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the existing recommendation is useful because it

  • Doesn't tell you which file to use
  • None of the existing code is enforced to the style guide, so it would cause unrelated changes

Proposing a reformat of the codebase was rejected in dev call because it would make existing PR's and backports much more difficult to rebase. Thus, I would propose recommending making new files astyle clean, and then we document that in the wiki.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what

* Doesn't tell you which file to use

you mean ? if you think that is an issue we could clarify what refers to

* None of the existing code is enforced to the style guide, so it would cause unrelated changes

its true when run on the file you have modified it would correct existing format issues....maybe we could start allowing an unsquashed ending commit in PRs for style changes to allow any new touched file to be astyle corrected?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The devs have been very clear they do not want noise on PR's for style changes. What I am trying to say (not effectively I guess) is that the existing wiki instructions have no relation to our current development practices. From what I can tell, no one manually runs astyle on existing code using the method in the wiki.

I've proposed many options to improve the development practices and reduce the time developers spend nitpicking formatting. Hardly any of my proposals have been accepted by the development team, except for the python script which works wonderfully so far and the CI task for enforcing it.

Hence, I want to document it, because it's working well, and I truly hope other developers writing new libraries get on board with automatic formatting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants